-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add supported platform section to README #481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. However, in this case I think we need to add a statement like:
The ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR.
Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks.
These mean that the library should be compatible with nearly any recent browser, on the majority of platforms.
If, however, you do find compatibility issues with any specific platform and browser combination, please raise an issue and/or contact customer support. Known compatibility issues can be found at ....
README.md
Outdated
|
||
**Note**: the ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR. | ||
Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks. | ||
These mean that the library should be compatible with nearly any recent browser, on the majority of platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compatible with nearly any recent browser
Do we not mean nearly every browser being used? Recent makes it sound like we're only supporting ones released in the last few months / year at best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just remove "recent"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove "recent"
We still need to do this
README.md
Outdated
| 8 | Firefox 58+ | | ||
| 9 | Safari macOS 11 | | ||
| | Safari iOS 11 | | ||
| | Chrome on Android 6| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overly restrictive -- I think we're underselling ourselves.
E.g. it makes it sound like we don't support firefox <=57, which may be rather off-putting for someone on, say, the current extended-support release (52). Or safari on iOS 10, or Chrome on Android 8, or the default browser on a Samsung phone, etc. etc. etc.
In reality we we do support all of those, in the sense that if someone found a bug in one of them we would put effort into reproducing and fixing it. We just don't test them on ci. But that's just for practical reasons -- if we could test every one of the thousands of platforms we consider 'supported' in that sense we would; since we can't, we test a selection of common ones. But that doesn't mean the sdk isn't compatible with others.
ISTM what's relevant to the customer is what we support, in that sense (what we'll accept and fix bug reports on), not what we happen to test in ci (which we reserve the right to change at any point without notice). So I'd suggest making the list of what we support be that. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonWoolf I have a similar concern.
We still have to extra time on the CI (before we max out the allowed session length). So we could add some older browsers like:
- Firefox 52
- Chrome TDB
- Safari iOS10
- Chrome on Android 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, we can discuss that in the CI issue. my main point for the purposes of this PR is that the libraries we support is a (large) superset of the libraries we test in ci, and we shouldn't imply otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that addressed with my comments re fallbacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. ISTM that having a big table at the top saying "we're only compatible with firefox 58+, chrome on Android 4.0, ..." just because those are the versions we currently test in ci is just misleading in a way that isn't really anything to do with whether we have transport fallbacks (it's not like firefox 57 doesn't support websockets). Rather it's to do with whether we'll fix bugs reported against firefox 57. Which we will, even through we don't have that specific version in ci for practicality reasons. No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @SimonWoolf's comments here. A visitor is probably not going to read the detail in reality, they are going to scan, see a table, and assume we don't support Opera or old versions, when we do.
Let's resume this conversation after we have added more browser tests on Browserstack |
Should we not resolve this now and add more browsers once #493 is fixed. We should avoid coupling an update of the README to a task that will simply improve that README IMHO. |
@mattheworiordan @paddybyers @SimonWoolf ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
README.md
Outdated
|
||
**Note**: the ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR. | ||
Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks. | ||
These mean that the library should be compatible with nearly any recent browser, on the majority of platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove "recent"
We still need to do this
@paddybyers Ooops. Fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
I don't think my and Matt's issues with this approach (#481 (comment)) have really been addressed. Rather than going over those again, I'll do my own version as a counterpoint and push it up to a branch, see what you think. |
See #498 for my proposal for this ( https://github.com/ably/ably-js/blob/c658b5ae76c49ef65026cf21a48abb1762a25504/README.md#supported-platforms ) |
lgtm |
No description provided.